Safely migrate to FileSystemStoreV2#872
Safely migrate to FileSystemStoreV2#872benthecarman wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
I've assigned @tnull as a reviewer! |
Before moving to PaginatedKVStore everywhere we need to use FileSystemStoreV2 instead of FileSystemStoreV1. This will safely migrate over to it on first start up. Also adds a test to make sure we handle it properly.
benthecarman
force-pushed
the
filesystem-v2
branch
from
f66c589 to
d90cc93
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Hmm, I have to say for the file system store I am a bit hesitant to just kick-off auto-migration like that, given users might have messed with the store in some manual fashion (def. had users do that before)?
| /// If the directory contains v1 data (files at the top level), the data is migrated to v2 format | ||
| /// in a temporary directory, the original is renamed to `fs_store_v1_backup`, and the migrated | ||
| /// directory is moved into place. | ||
| fn open_or_migrate_fs_store(storage_dir_path: PathBuf) -> Result<FilesystemStoreV2, BuildError> { |
There was a problem hiding this comment.
Let's move this to io/utils.rs
| match FilesystemStoreV2::new(storage_dir_path.clone()) { | ||
| Ok(store) => Ok(store), | ||
| Err(e) if e.kind() == std::io::ErrorKind::InvalidData => { | ||
| // The directory contains v1 data, migrate to v2. |
There was a problem hiding this comment.
Huh, how do we know io::ErrorKind::InvalidData corresponds to v1? Couldn't it have other reasons (eg if users misconfigured something) and we might be screwing with users data here?
| let mut backup_dir = storage_dir_path.clone(); | ||
| backup_dir.set_file_name("fs_store_v1_backup"); | ||
| fs::rename(&storage_dir_path, &backup_dir) | ||
| .map_err(|_| BuildError::KVStoreSetupFailed)?; |
There was a problem hiding this comment.
Note that if we crash here the user can't recover. Can we somehow make the two renames part of the same fsync/metadata operation? Not sure?
| async fn build_0_7_0_node( | ||
| bitcoind: &BitcoinD, electrsd: &ElectrsD, storage_path: String, esplora_url: String, | ||
| seed_bytes: [u8; 64], | ||
| seed_bytes: [u8; 64], use_fs_store: bool, |
There was a problem hiding this comment.
No, please have this take and use TestConfig for stuff like that, all the bool flags in test code are getting hard to read.
3 participants
Before moving to PaginatedKVStore everywhere we need to use FileSystemStoreV2 instead of FileSystemStoreV1. This will safely migrate over to it on first start up. Also adds a test to make sure we handle it properly.